Conversation
MabezDev
left a comment
MabezDev
left a comment
There was a problem hiding this comment.
Thanks, I haven't tested it, but I'm sure it works :D. I've added some comments which I think will improve the PR.
espflash/src/connection/mod.rs
Outdated
| 4 | ||
| }; | ||
|
|
||
| let status_len = |
There was a problem hiding this comment.
This could benefit from some comments, and perhaps a permalink to either the esptool impl, or the docs explaining this return data from the ROM bootloader.
| pub features: Vec<String>, | ||
| /// MAC address | ||
| pub mac_address: String, | ||
| pub mac_address: Option<String>, |
There was a problem hiding this comment.
The ROM bootloader doesn't send MAC in SDM?
There was a problem hiding this comment.
Yeah, the MAC is read from efuse, but in SDM you can't read efuses, so that's the reason. I should probably add a comment to clarify this though, thanks!
espflash/src/flasher/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn detect_sdm(connection: &mut Connection) -> Result<bool, Error> { |
There was a problem hiding this comment.
Can we detect sd mode in the normal detection routine, instead of doing it twice?
There was a problem hiding this comment.
I.e, inside connection.begin(), can we return the download mode type there?
espflash/src/flasher/mod.rs
Outdated
| if use_stub { | ||
| info!("Using flash stub"); | ||
| flasher.load_stub()?; | ||
| if !sdm { |
There was a problem hiding this comment.
If you bring the !sdm if outside, you can avoid two seperate if's here and below
| _segment: Segment<'_>, | ||
| _progress: &mut Option<&mut dyn ProgressCallbacks>, | ||
| ) -> Result<(), Error> { | ||
| todo!() |
There was a problem hiding this comment.
If we can't implement this, this should return a nice error, not panic.
There was a problem hiding this comment.
If it can be implemented at some point, please also add a TODO with a relevant issue.
espflash/src/flasher/mod.rs
Outdated
|
|
||
| let revision = Some(target.chip_revision(self.connection())?); | ||
| // chip_revision reads from efuse, which is not possible in Secure Download Mode | ||
| let revision = if !self.connection.secure_download_mode { |
There was a problem hiding this comment.
We can use https://doc.rust-lang.org/std/primitive.bool.html#method.then_some to remove the need for the if statements.
There was a problem hiding this comment.
@MabezDev
Not "then_some", as it eagerly evaluates arguments inside of it, which means, that even if chip IS in Secure Download Mode, "read_reg" inside of that "chip_revision" function will be evaluated -> everything will fail.
Instead, me might want to use something like
let revision = (!self.connection.secure_download_mode)
.then(|| target.chip_revision(self.connection()))
.transpose()?;
CHANGELOG.md
Outdated
| - Make the default flashing frequency target specific (#389) | ||
| - Add note about permissions on Linux (#391) | ||
| - Add a diagnostic to tell the user about the partition table format (#397) | ||
| - Add (some level of) SDM support (#832) |
There was a problem hiding this comment.
Would be nice to have some details here, not really sure what this means TBH.
| } else { | ||
| Command::SpiAttach { | ||
| spi_params: self.spi_attach_params, | ||
| if connection.secure_download_mode { |
There was a problem hiding this comment.
Would be cleaner to just use else if here rather than nested if statements, IMO.
|
I've found a great bug there, will get back to it later |
|
Thank you @MabezDev and @jessebraham for the reviews though, will address them when I'm back to this task. |
* Cut off the write_bin part from original #832 * Changelog entry * Akela missed... * Dumb * Address reviews
fmt clean yeah
3 participants
closes #726
Initial issue contains just a little bit of lie: espflash didn't work with SDM almost at all 😅
After this is merged, at least board-info and write-bin (mentioned in the original issue) functions will be supported.
No extra flags are needed, user will be able to just execute:
espflash write-bin 0x0 --monitor <bin-name>I recommend setting a higher baudrate as otherwise flashing in SDM takes a lot of time.
Also,
--no-stubis applied automatically when tool detects that connected chip is in SDM mode.